-
-
Notifications
You must be signed in to change notification settings - Fork 2k
Resolve some suspense crashes #4999
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
📊 Tachometer Benchmark ResultsSummaryduration
usedJSHeapSize
Resultscreate10kduration
usedJSHeapSize
filter-listduration
usedJSHeapSize
hydrate1kduration
usedJSHeapSize
many-updatesduration
usedJSHeapSize
replace1kduration
usedJSHeapSize
run-warmup-0
run-warmup-1
run-warmup-2
run-warmup-3
run-warmup-4
run-final
text-updateduration
usedJSHeapSize
tododuration
usedJSHeapSize
update10th1kduration
usedJSHeapSize
|
|
Size Change: +80 B (+0.1%) Total Size: 79.3 kB
ℹ️ View Unchanged
|
| }); | ||
| }); | ||
|
|
||
| it('should not crash when suspended child updates after unmount', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Uncommenting line 240 will surface the crash of this
6363cef to
c4514f6
Compare
Before this when we unmounted a vnode which was in a suspended state it was possible to cause this to render while being unmounted. One disturbing thing is that I wanted to check vnode._component._onResolve in the options.unmount hook, this however was not possible because somehow the reference to `_component` isn't being updated on the vnode that's actually unmounting. For this reason I opted to use a separate flag.
marvinhagemeister
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
| } from 'preact/compat'; | ||
| import { setupScratch, teardown } from '../../../test/_util/helpers'; | ||
| import { createLazy, createSuspender } from './suspense-utils'; | ||
| import { expect } from 'chai'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe nit: is this needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whups, darn vscode auto-import
* Fix suspense crash * Prevent unmounted suspended vnodes from rendering Before this when we unmounted a vnode which was in a suspended state it was possible to cause this to render while being unmounted. One disturbing thing is that I wanted to check vnode._component._onResolve in the options.unmount hook, this however was not possible because somehow the reference to `_component` isn't being updated on the vnode that's actually unmounting. For this reason I opted to use a separate flag.
In the first commit we are resolving a suspense crash that happens when we do a state update on a component that is in a suspended state which has been unmounted.
In the second commit we resolve a forced remount of an unmounted component which resolves after unmount.
RE the second commit
Before this when we unmounted a vnode which was in a suspended state it was possible to cause this to render while being unmounted.
One disturbing thing is that I wanted to check
vnode._component._onResolvein theoptions.unmounthook, this however was not possible because somehow the reference to_componentisn't being updated on thevnode(my assumption is it being related to us cloning the vnode during the suspension) that's actually unmounting. For this reason I opted to use a separate flag.